Migrate crop config creator node #191

aljazkonec1 · 2025-03-17T22:53:40Z

Purpose

This PR migrates the ProcessDetections node, renamed to CropConfigCreatorNode, from depthai-experiments to depthai-nodes. The node simplifies the pipeline by removing one extra script node and expands the usability as it unblocks RVC2 usage. The naming of the node and any other suggestions are kindly welcome!

Specification

Add a new node, called CropConfigCreatorNode. This node is used to create a dai.ImageManipConfigV2() object for every detection. The node works on both RVC2 and RVC4. Simultaneously, this node also addresses the issue with RVC2 with pool size that has been blocking depthai-experiments.

Dependencies & Potential Impact

No breaking changes outright, but an update to depthai-experiments will be needed. One risk is the way the node handles syncing of which crop config is for which frame. The node assumes the last frame is saved in the ImgManipV2 node and when recieving a detection_message it sends an empty crop config that skips the frame and loads the next frame in the queue. That frame should be the one from which the detection_message was gotten.

Deployment Plan

None / not applicable

Testing & Validation

Tested on age-gender experiment on this branch on both RVC2 and 4 for 0 to 6 faces present in the frame. No unsynced frames could be detected. PS: pay attention to how the node is used in main.py and how the following ImageManipNode (script_node) has to be set up in order for the correct sequence.

codecov-commenter · 2025-03-17T22:55:18Z

Codecov Report

Attention: Patch coverage is 97.97980% with 2 lines in your changes missing coverage. Please review.

Project coverage is 48.68%. Comparing base (91bd184) to head (ec7ce9e).

✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
depthai_nodes/node/host_crop_config_creator.py	97.95%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##           main     #191       +/-   ##
=========================================
+ Coverage      0   48.68%   +48.68%     
=========================================
  Files         0       76       +76     
  Lines         0     4404     +4404     
=========================================
+ Hits          0     2144     +2144     
- Misses        0     2260     +2260

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

kkeroo

LGTM otherwise. Please also add unit test for the node.

depthai_nodes/node/host_crop_config_creator.py

jkbmrz · 2025-03-18T07:19:23Z

depthai_nodes/node/host_crop_config_creator.py

+        setReusePreviousImage flag set to False, which changes the previous frame for a
+        new one.
+        """
+        assert isinstance(detections_input, ImgDetectionsExtended)


should we add here ... or isinstance(detections_input, dai.ImgDetection)? Might come handy if e.g. YOLO is the first model of the pipeline

I would preffer not to add it, because then the eintire node would have to be split into if ImgDetectionsExtended: ... elif dai.ImgDetections: .... I would rather have YOLO models be updated to ImgDetectionsExtended instead of using ImgDetections.

I think that (sadly) update to dai YOLO parser using ImgDetectionsExtended is quite a bit of a way out on the roadmap. So I would agree that algthough a bit PITA we should make the node work with both ImgDetections and ImgDetectionsExtended

Okay I'll add support for ImgDetections but the more we account for dai.ImgDetections the later we will migrate YOLO parser as it wont be so "critical"

jkbmrz · 2025-03-18T07:26:37Z

depthai_nodes/node/host_crop_config_creator.py

+
+        for i in range(num_detections):
+            cfg = dai.ImageManipConfigV2()
+            detection: ImgDetectionExtended = detections[i]


Are the detections sorted by confidence? Because if not, we might not send out the desired ones (e.g. if a model makes 300 detections, and we limit them to 100, then it would make sense to output the top 100 most confident ones).

The detections are sorted by which ever parser created the ImgDetectionsExtended message. I thought about adding a sorting, but it seems a bit out place given this is just a config creator node. You dont know if the user does any other types of syncing with the detections and sorting them could completely throw off the sync

Agree, I would leave any sorting out fo this node as it should only do the crop config

Giving it a second thought, why do we even want to limit the amount of detections here? We are planning to also add a DetectionsFilter node that will filter the provided detections (e.g. by class, confidence, etc.) and we can maybe also add a number limitation there.

I added it as like an extra buffer to limit amount of outgoing messages in case the models are large or the device struggles processing all detections or the visualization is crammed with visuals. I wasn't aware about the DetectionsFilter node, so when its implemented I can remove n_detections?

I think we can remove already, no need to wait for DetectionsFilter node and then do a change in the implementation of this one.

klemen1999

Let's also add unittests

kkeroo · 2025-03-24T08:36:09Z

@aljazkonec1 can you also adjust the unit test so it could be used in stability tests as well? The adjustments are simple.

You need to add fixture duration like here.
Add duration parameter to each test function like here
Add a check in each test function if duration is set and then perform the neccessary checks and validation inside if statement like here.

This would be then compatible with stability tests and the workflow could take this node also into the stability tests pipeline.

You can check if it works by running pytest --duration 10 in the tests folder to run the tests with 10 second duration.

jkbmrz

Generally LGTM but yet adding two small comments.

jkbmrz · 2025-03-24T09:07:40Z

depthai_nodes/node/host_crop_config_creator.py

+from depthai_nodes import ImgDetectionExtended, ImgDetectionsExtended
+
+
+class CropConfigsCreatorNode(dai.node.HostNode):


Should we rename the node from CropConfigsCreatorNode to CropConfigsCreator? It would better follow the naming of other nodes as none of them have “Node” in their name (they are "nodes" by default).

+1 on the new name

Good catch, I'll change it.

depthai_nodes/node/host_crop_config_creator.py

PetrNovota · 2025-03-24T12:04:19Z

depthai_nodes/node/host_crop_config_creator.py

+    target_h : int
+        The height of the target image.
+    n_detections : int
+        The number of detections to keep.


put the default value in here as well

PetrNovota · 2025-03-24T12:12:13Z

depthai_nodes/node/host_crop_config_creator.py

+    will keep at most the first 100 detections.
+
+    Before use, the source and target image sizes need to be set with the build function.
+    The node assumes the last frame is saved in the dai.ImgManipV2 node and when recieving a detection_message the node sends an empty crop config that skips the frame and loads the next frame in the queue.


why do we need this assumption? How can we get rid of it?
Wouldn't it be better if this node also accepted the image and then it could make sure the crops are always correctly created?

Problem is that this is a host node and is run on host. Recieving a frame from device and then sending n frames back to the device completely fills the network and slows the pipeline considerably. And we cant make this a script node because there are no ImgDetectionsExtended on the devices (yet).

huh, i see. this is going to be tough to make the host nodes work in both the peripheral and local modes.
I remember there was a ImageManip config, something like wait_for_image and wait_for_config. wait_for_config ment that if the manipulation has an image ready, it has to wait for a manip cfg before processing it. Mabe if this is still the case in depthaiV3 requestion the manipulation to be configured like this would be better dependency then doing this workaround which will sometimes fail?

PetrNovota · 2025-03-24T12:12:42Z

depthai_nodes/node/host_crop_config_creator.py

+        The output link for the ImageManipConfigV2 messages
+    detections_output : dai.Output
+        The output link for the ImgDetectionsExtended message
+    w : int


why cannot we work with relative coordinates?

When there is a rotated rectangle, cropping in relative coordinates creates a sheared img crop in the original coordinates due to the aspect ratio in relative coordinates being 1:1 while in absolute coordinates the aspect ratio can be anything (16:9, 3:2, etc.)

isn't it a bug in depthai? I don't see why it matters if the bbox is rotated or not.

PetrNovota · 2025-03-24T12:14:56Z

depthai_nodes/node/host_crop_config_creator.py

+        The height of the source image.
+    target_w : int
+        The width of the target image.
+    target_h : int


I don't understand what does this do. I can see it's used here: cfg.setOutputSize(self.target_w, self.target_h)
But I don't understand it's purpose. Is this a command to resize the crop? If so, what if I don't want to resize it?

Yes, because the size of the detected rectanlge can be anything, this is so that the output of crop node is always the same so the cropped images can be used in a second NN for example.

Now that you mention it, I doo see that its an assumption that someone will want the same size outputs. I can change it to be an option (eg. equalize_crop_size = True). Thoughts @klemen1999

In the minemaestro app, we were generating crops based on bboxes and we did not resize, unless the resulting crop was then a threshold. So for me, a general node would have these options:

crop and keep the resulting size

crop and set a fixed size. with this, we would need the options to set different resizing options like: stratch or letterbox

crop and resize if resulting size is bigger then threshold

PetrNovota · 2025-03-24T12:16:40Z

depthai_nodes/node/host_crop_config_creator.py

+        cfg.setSequenceNum(sequence_num)
+        send_status = False
+        while not send_status:
+            send_status = self.config_output.trySend(cfg)


As mentioned above, seems like a hack which will break in some situations and then its going to be a mess. Or does it work every time?
Also, if this while loop is needed, maybe add some small wait to prevent spamming the trySend?

trySend already waits for connected outputs to recieve the message before returning the boolean so there is no spamming of messages. In theory this means there should be no miss-sync (at least not due to this while loop)

ok. from the dai's tryGet, which doesn't wait, I was under the impression that this would immediately return False in case the message cannot be sent.

aljazkonec1 added 3 commits March 17, 2025 23:29

Add CropConfigsCreatorNode

bb01fe3

Add CropCreatorNode to init

5da3eed

small change to logic

7e2e279

aljazkonec1 requested review from tersekmatija, jkbmrz, kkeroo and klemen1999 as code owners March 17, 2025 22:53

github-actions bot assigned aljazkonec1 Mar 17, 2025

aljazkonec1 requested a review from dominik737 March 17, 2025 22:54

kkeroo reviewed Mar 18, 2025

View reviewed changes

depthai_nodes/node/host_crop_config_creator.py Show resolved Hide resolved

depthai_nodes/node/host_crop_config_creator.py Show resolved Hide resolved

jkbmrz reviewed Mar 18, 2025

View reviewed changes

klemen1999 reviewed Mar 19, 2025

View reviewed changes

aljazkonec1 added 3 commits March 21, 2025 18:08

Partial Unit Test finished

786bc39

Merge branch 'main' into migrate-crop-config-creator-node

de24788

All unit tests for Config Creator node

ec7ce9e

jkbmrz reviewed Mar 24, 2025

View reviewed changes

PetrNovota requested changes Mar 24, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate crop config creator node #191

Migrate crop config creator node #191

aljazkonec1 commented Mar 17, 2025

codecov-commenter commented Mar 17, 2025 •

edited

Loading

kkeroo left a comment

jkbmrz Mar 18, 2025

aljazkonec1 Mar 22, 2025

klemen1999 Mar 24, 2025

aljazkonec1 Mar 24, 2025

jkbmrz Mar 18, 2025

aljazkonec1 Mar 22, 2025

klemen1999 Mar 24, 2025

jkbmrz Mar 24, 2025 •

edited

Loading

aljazkonec1 Mar 24, 2025

klemen1999 Mar 24, 2025

klemen1999 left a comment

kkeroo commented Mar 24, 2025 •

edited

Loading

jkbmrz left a comment •

edited

Loading

jkbmrz Mar 24, 2025

klemen1999 Mar 24, 2025

aljazkonec1 Mar 24, 2025

PetrNovota Mar 24, 2025

PetrNovota Mar 24, 2025

aljazkonec1 Mar 24, 2025 •

edited

Loading

PetrNovota Mar 24, 2025

PetrNovota Mar 24, 2025

aljazkonec1 Mar 24, 2025

PetrNovota Mar 24, 2025

PetrNovota Mar 24, 2025

aljazkonec1 Mar 24, 2025

aljazkonec1 Mar 24, 2025

PetrNovota Mar 24, 2025

PetrNovota Mar 24, 2025

aljazkonec1 Mar 24, 2025

PetrNovota Mar 24, 2025

		from depthai_nodes import ImgDetectionExtended, ImgDetectionsExtended


		class CropConfigsCreatorNode(dai.node.HostNode):

Migrate crop config creator node #191

Are you sure you want to change the base?

Migrate crop config creator node #191

Conversation

aljazkonec1 commented Mar 17, 2025

Purpose

Specification

Dependencies & Potential Impact

Deployment Plan

Testing & Validation

codecov-commenter commented Mar 17, 2025 • edited Loading

Codecov Report

kkeroo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkbmrz Mar 24, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

klemen1999 left a comment

Choose a reason for hiding this comment

kkeroo commented Mar 24, 2025 • edited Loading

jkbmrz left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aljazkonec1 Mar 24, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Mar 17, 2025 •

edited

Loading

jkbmrz Mar 24, 2025 •

edited

Loading

kkeroo commented Mar 24, 2025 •

edited

Loading

jkbmrz left a comment •

edited

Loading

aljazkonec1 Mar 24, 2025 •

edited

Loading